Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] UI Inventory #1755

Merged
merged 5 commits into from
Jul 30, 2018
Merged

[WIP] UI Inventory #1755

merged 5 commits into from
Jul 30, 2018

Conversation

pinkiebell
Copy link
Contributor

@pinkiebell pinkiebell commented Jul 15, 2018

#1738

  • Alerts
  • Animations/Loaders
  • Buttons
  • Forms
  • Typography (H1-H6, body copy, etc)
  • Search Field
  • Color Swatches
  • Iconography
  • Collapse/Expanders
  • Images (in various sizes)
  • Cards
  • Carousels
  • Checkboxes
  • Button Groups
  • Dropdown Menus
  • Data Tables
  • Illustrations (show all)
  • Page Headers (show an example of each)
  • Dialogs

@@ -939,3 +939,11 @@ def tokens(request):
key = f"{network}_tokens"
context[key] = Token.objects.filter(network=network, approved=True)
return TemplateResponse(request, 'tokens_js.txt', context, content_type='text/javascript')

def ui(request):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E302 expected 2 blank lines, found 1

@owocki
Copy link
Contributor

owocki commented Jul 16, 2018

let me know when this isnt WIP anymore and ill review

@codecov
Copy link

codecov bot commented Jul 16, 2018

Codecov Report

Merging #1755 into master will decrease coverage by 0.04%.
The diff coverage is 10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1755      +/-   ##
==========================================
- Coverage   28.58%   28.53%   -0.05%     
==========================================
  Files         128      128              
  Lines       10066    10089      +23     
  Branches     1328     1334       +6     
==========================================
+ Hits         2877     2879       +2     
- Misses       7088     7109      +21     
  Partials      101      101
Impacted Files Coverage Δ
app/app/urls.py 90% <ø> (ø) ⬆️
app/retail/views.py 29.48% <10%> (-1.83%) ⬇️
app/retail/utils.py 14.9% <0%> (-0.22%) ⬇️
app/dashboard/helpers.py 17.56% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e9731c...bf4447c. Read the comment docs.

var last = null;

while (buttons !== null) {
function onClick(evt) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move function declaration to function body root. (no-inner-declarations)
Don't make functions within a loop. (no-loop-func)

while (buttons !== null) {
function onClick(evt) {
var width = evt.target.offsetWidth;
var offset = evt.target.offsetLeft;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected blank line after variable declarations. (newline-after-var)

indicator.style.width = width + 'px';
indicator.style.transform = 'translateX(' + offset + 'px)';

var current = sections.querySelector('#' + evt.target.id);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected blank line after variable declarations. (newline-after-var)

@pinkiebell
Copy link
Contributor Author

@owocki Ready for review.
If I forgot a UI element, let me know.

@pinkiebell pinkiebell changed the title WIP: UI Inventory UI Inventory Jul 16, 2018
app/app/urls.py Outdated
@@ -178,6 +178,7 @@
# brochureware views
url(r'^about/?', retail.views.about, name='about'),
url(r'^mission/?', retail.views.mission, name='mission'),
url(r'^ui/?', retail.views.ui, name='ui'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use re_path since we are trying to migrate completely from url

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the requested changes, can you please include an addition to the CONTRIBUTING.md document explaining any addition of visual assets should be included in the inventory?

{% endcomment %}
{% load i18n static %}
<!DOCTYPE html>
<html lang="en">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you wrap relevant copy with {% trans "" %} ?

app/app/urls.py Outdated
@@ -178,6 +178,7 @@
# brochureware views
url(r'^about/?', retail.views.about, name='about'),
url(r'^mission/?', retail.views.mission, name='mission'),
url(r'^ui/?', retail.views.ui, name='ui'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

@mbeacom mbeacom changed the title UI Inventory [WOC] UI Inventory Jul 16, 2018
@mbeacom mbeacom added frontend This needs frontend expertise. waiting on contributor and removed waiting on contributor labels Jul 16, 2018
@mbeacom mbeacom changed the title [WOC] UI Inventory UI Inventory Jul 16, 2018
@thelostone-mc
Copy link
Member

@PixelantDesign @mbeacom @SaptakS

This is the how the page looks at the moment

screenshot_2018-07-18 ui inventory gitcoin

@pinkiebell
Copy link
Contributor Author

@thelostone-mc
The two robo-heads in the animation section and the content in the carousel section is missing in the screenshot.

@thelostone-mc
Copy link
Member

thelostone-mc commented Jul 18, 2018

@pinkiebell lol thats cause I took a full page screenshot and the animation kicks in only when you actually scroll :P

Btw just synced up with @PixelantDesign

  • Could you tick whatever you've covered on the checklist (mentioned in the issue)
  • Also could you throw in the code snippet below each component so that folks know how to use it
    (don't worry about how it looks, we'll fix it up in iterative-ly )
  • resolve conflicts by rebasing with master

@pinkiebell
Copy link
Contributor Author

pinkiebell commented Jul 18, 2018

@thelostone-mc

Can you checkout the new changes?
I did a rebase, and included the modal dialog and you can now click on the examples on the page to open a modal dialog with the 'showdown'-converted html source 👍 .

screenshot 2018-07-18 at 19 28 12

@thelostone-mc
Copy link
Member

thelostone-mc commented Jul 19, 2018

@pinkiebell wohoo ^_^

x1

@PixelantDesign Merge ready ?
@pinkiebell has checked out whatever she's added in the list

@SaptakS / @mbeacom If in case you guys merge it before I get to it -> we need to change to uri to /styleguide-alpha so that it's hard to find :P (As advised by @PixelantDesign )

thelostone-mc
thelostone-mc previously approved these changes Jul 19, 2018
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SaptakS
Copy link
Contributor

SaptakS commented Jul 20, 2018

I am just curious if giving an icons like here to show the code is more intuitive than clicking on the card.

pinkiebell added 3 commits July 21, 2018 03:58
Improve the code viewer and give the user a hint.
@pinkiebell
Copy link
Contributor Author

pinkiebell commented Jul 21, 2018

Just a quick rebase and I added some more assets + style fixes.
@SaptakS
Isn't it a bit too much If we show a icon on every example?

PS: changed path to /styleguide-alpha

@PixelantDesign
Copy link
Contributor

Looks good thank you @pinkiebell


<div class="container-fluid p-5 assets">
<p class="h1">{% trans "Vector Graphics" %}</p>
<img src="{% static "v2/images/quickstart-box.svg" %}"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not saying we need to do it here... but we'll probably want to modify the vast majority of the asset handling and repetitive entries here to simply loop over assets in images/ and the associated subs / context data. We'd drop the line count in this file down by quite a bit and we wouldn't have to manually add every new asset as they're added (I'd be willing to bet it won't be long until we're missing assets here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be very nice. I'm not familiar with Django, so I will dig into that today. 👍

@mbeacom mbeacom changed the title UI Inventory [WIP] UI Inventory Jul 21, 2018
@mbeacom mbeacom added the wip label Jul 21, 2018
@thelostone-mc
Copy link
Member

@SaptakS meh it's just for a first pass -> we'll improve it in cycles
I guess the scope of this task was just to get all the components together on one page

I already bugged @pinkiebell to add the code snippets (even though I believe it wasn't part of the original task :P )

thelostone-mc
thelostone-mc previously approved these changes Jul 30, 2018
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@thelostone-mc thelostone-mc merged commit 8b2db5b into gitcoinco:master Jul 30, 2018
@pinkiebell pinkiebell deleted the inventory branch November 6, 2018 08:44
jvmaia pushed a commit to jvmaia/web that referenced this pull request Nov 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend This needs frontend expertise.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants